Hi @truongsontung!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@claude review this PR

Claude finished @tianyu-l's task in 1m 31s —— View job

Code Review: SSRF protection in image decoder

Thanks for the fix @truongsontung. The intent is right and the structure is clean, but as written the check has a few bypasses that mean it doesn't actually stop a determined SSRF. Details below.

🔴 Blocking issues

1. DNS rebinding / TOCTOU — the check is bypassable (image.py:52 + image.py:73)
_is_safe_url() resolves the hostname with socket.getaddrinfo(...), then requests.get(image, ...) resolves DNS again independently. An attacker controlling the DNS response can return a public IP for the check and a private IP (e.g. 169.254.169.254) for the actual fetch. This is the classic SSRF-filter bypass, so the validation gives a false sense of safety.

The robust fix is to resolve once, verify the resolved IP, and then connect to that pinned IP (passing the original Host header) rather than re-resolving. That's more involved — at minimum this limitation should be documented.

2. Redirects are not re-validated (image.py:73)
requests.get() follows redirects by default. A safe-looking public URL can 302 to http://169.254.169.254/latest/meta-data/, and requests will follow it with no re-check. You need either allow_redirects=False, or a custom adapter/session that re-runs _is_safe_url on every hop.

3. IPv6 and IPv4-mapped addresses are not covered (image.py:32-40)
_PRIVATE_IP_RANGES lists only IPv4 networks. getaddrinfo can return IPv6 results, so these slip through:

• ::1 (loopback), fe80::/10 (link-local), fc00::/7 (unique-local), :: (unspecified)
• ::ffff:127.0.0.1 (IPv4-mapped) — on many Python versions IPv6Address(...).is_loopback is False for this, so it passes the current in network check.

Rather than maintaining a hand-rolled list, lean on ipaddress properties which handle both families:

def _ip_is_blocked(ip: ipaddress._BaseAddress) -> bool:
    mapped = getattr(ip, "ipv4_mapped", None)
    if mapped is not None:
        ip = mapped  # unwrap ::ffff:a.b.c.d before classifying
    return (
        ip.is_private or ip.is_loopback or ip.is_link_local
        or ip.is_reserved or ip.is_multicast or ip.is_unspecified
    )

This also picks up ranges the current list misses, e.g. 100.64.0.0/10 (CGNAT) and 192.0.0.0/24. (169.254.169.254 is already covered by 169.254.0.0/16, good.)

Fix these →

🟡 Suggestions

• No test added. Per CLAUDE.md PR expectations, new features need at least a CPU unit test. A small pytest case for _is_safe_url (localhost, 169.254.169.254, an IPv4-mapped IPv6, a file:// scheme, and a normal https URL) would lock in the behavior and prevent regressions.
• str.startswith("http") also matches httpfoo:// (image.py:70). Minor, since _is_safe_url re-checks the scheme, but matching on the parsed scheme would be cleaner.
• Consider whether unrestricted outbound fetch belongs in the dataloader at all. If dataset images are expected to be local bytes/paths in practice, an allowlist (or a config flag to opt into URL fetching) would be a stronger posture than a denylist. Denylists for SSRF are notoriously leaky.

✅ What's good

• Scheme restricted to http/https.
• Metadata endpoint (169.254.169.254) and the common private/loopback IPv4 ranges are covered.
• Fail-closed on resolution errors (return False in the except).

Net: the approach is reasonable but the DNS-rebinding and redirect gaps make it bypassable today, and IPv6/mapped addresses are uncovered. I'd address 1-3 before merge.

(Note: CLA still needs signing per the bot comment above before this can merge.)
• branch fix/ssrf-image-decoder

@claude please review again

Claude finished @tianyu-l's task in 4m 14s —— View job

Re-review: SSRF protection in image decoder

Thanks for the follow-up @truongsontung. Good progress since the last round — issue #3 (IPv6) is substantially addressed. But one of the two blocking items is only apparently fixed: the redirect guard is wired to a hook requests never calls, so redirects are still followed unchecked.

🔴 Blocking

1. The redirect guard is dead code — redirects are still unvalidated (image.py:69-77, 92)

_SSRFProtectedAdapter overrides resolve_redirects, but that method lives on requests's SessionRedirectMixin (inherited by Session), not on HTTPAdapter. HTTPAdapter only defines send, build_response, get_connection, cert_verify, etc. Redirect resolution happens in Session.send() -> self.resolve_redirects(...) at the session level, so your adapter override is never invoked. Net effect: with allow_redirects=True (line 92), a safe-looking public URL can still 302 -> http://169.254.169.254/latest/meta-data/ and it will be followed with no re-check. This is exactly the gap flagged last time, just relocated.

Two clean ways to actually close it:

# Option A: subclass Session (resolve_redirects is a Session method)
class _SSRFProtectedSession(requests.Session):
    def resolve_redirects(self, resp, req, **kwargs):
        loc = resp.headers.get("Location")
        if loc and not _is_safe_url(urljoin(resp.url, loc)):
            raise requests.exceptions.InvalidURL(f"Blocked redirect: {loc}")
        return super().resolve_redirects(resp, req, **kwargs)

# Option B: disable redirects and loop manually, re-validating each hop
resp = session.get(image, timeout=10, allow_redirects=False)
while resp.is_redirect:
    nxt = urljoin(resp.url, resp.headers["Location"])
    if not _is_safe_url(nxt):
        raise ValueError(f"Blocked redirect: {nxt}")
    resp = session.get(nxt, timeout=10, allow_redirects=False)

Note both still resolve the redirect target's Location as a URL, so relative redirects need urljoin against resp.url — a bare _is_safe_url("/path") returns False on a relative target and would wrongly reject a legitimate relative redirect. Fix this →

🟡 Still open from last review

2. DNS rebinding / TOCTOU (image.py:55 + 92) — not addressed. _is_safe_url() resolves via getaddrinfo, then session.get(image, ...) resolves DNS independently a second time. An attacker's DNS can return a public IP for the check and 169.254.169.254 for the fetch. The robust fix is resolve-once-then-pin (connect to the validated IP with the original Host header). That's more involved — if you don't want to implement pinning now, at minimum add a comment documenting this as a known limitation so it isn't mistaken for airtight.

3. No test added. Per CLAUDE.md PR expectations, new features need at least a CPU unit test. A small pytest for _is_blocked_ip / _is_safe_url (localhost, 169.254.169.254, an IPv6 loopback ::1, a file:// scheme, and a normal https URL) would lock in behavior. _is_safe_url does live DNS, so mock socket.getaddrinfo — that also lets you assert the redirect path once fixed.

🟢 Minor

• is_reserved dropped from _is_blocked_ip (image.py:36-42). The current set covers private/loopback/link-local/multicast/unspecified but not is_reserved (e.g. 240.0.0.0/4). Cheap to add alongside the others.
• IPv4-mapped IPv6 (::ffff:a.b.c.d) not unwrapped. Low practical risk here since getaddrinfo without AI_V4MAPPED returns real A/AAAA records rather than mapped forms, but unwrapping ip.ipv4_mapped before classifying would make _is_blocked_ip correct regardless of caller.
• startswith("http") (image.py:86) still matches httpfoo://; harmless because _is_safe_url re-checks the parsed scheme, but matching on the parsed scheme reads cleaner.

✅ Improved since last round

• IPv6 / link-local / multicast / unspecified now covered via ipaddress properties (_is_blocked_ip) — much better than the hand-rolled IPv4 list.
• All resolved IPs checked, not just the first (any(...) over getaddrinfo results).
• Fail-closed on resolution errors.
• Scheme restricted to http/https; explicit timeout.

Net: the IPv6 hardening landed well, but the redirect protection is currently a no-op because it hooks the wrong class — that's the one I'd block on. DNS-rebinding and a test are the remaining follow-ups.

(CLA still needs signing per the bot comment above before this can merge.)

Note: I couldn't execute requests in this sandbox to demonstrate the dead-code path at runtime; the finding is based on the requests API (resolve_redirects is defined on SessionRedirectMixin, not HTTPAdapter). Worth a quick local confirmation on your end.
• branch fix/ssrf-image-decoder

@claude review again

Claude finished @tianyu-l's task in 3m 37s —— View job

Re-review: SSRF protection in image decoder

Thanks for the update @truongsontung. The dead-code problem from last round is genuinely fixed now — but the redirect guard as written only validates the first hop, so a multi-hop redirect chain still slips through. Details below.

🟢 Resolved since last round

Redirect hook now actually fires (image.py:66-75, 88). Moving the override onto a requests.Session subclass is correct: resolve_redirects is defined on SessionRedirectMixin, which Session inherits, and Session.send() invokes self.resolve_redirects(...). So _SSRFProtectedSession.resolve_redirects is called now — the previous no-op (it was on HTTPAdapter) is fixed. Good catch on the commit message.

🔴 Blocking — only the first redirect hop is validated (image.py:69-75)

resolve_redirects is not called once per hop — it's a generator that loops the entire redirect chain internally (while url: ... resp = self.send(prepared_request, allow_redirects=False); url = self.get_redirect_target(resp); yield resp). Your override checks resp.headers.get("Location") on the initial response (hop 1's target) and then hands the whole chain to super().resolve_redirects(...), which follows hops 2, 3, ... with no re-validation.

So this bypass still works:

https://attacker.example/img.png   (safe public IP -> passes _is_safe_url + hop-1 check)
  -> 302 https://attacker.example/step2   (safe public IP -> hop-1 check passed, this is where super() takes over)
  -> 302 http://169.254.169.254/latest/meta-data/   (fetched by super(), never checked)

The first hop is guarded; every hop after it is not. (This is actually a flaw in the "Option A" I suggested last time — subclassing Session fixes where the hook lives, but a single resolve_redirects call still drives the full chain, so guarding only the entry response isn't enough. My earlier suggestion was wrong on that point.)

Note you can't fix this by iterating the parent generator and checking each yielded redirect_resp.url either — by the time super() yields a hop, it has already sent the request to that IP, so the SSRF has already happened. The check must run before each send.

The robust fix is the manual loop (validate every target before fetching it):

resp = session.get(image, timeout=10, allow_redirects=False)
while resp.is_redirect:
    nxt = urljoin(resp.url, resp.headers["Location"])  # urljoin handles relative Location
    if not _is_safe_url(nxt):
        raise ValueError(f"Blocked redirect to unsafe URL: {nxt}")
    resp = session.get(nxt, timeout=10, allow_redirects=False)
image = resp.content

With this, _SSRFProtectedSession / the resolve_redirects override can be dropped entirely. Fix this →

🟡 Still open from last review

2. DNS rebinding / TOCTOU (image.py:52 + 88) — not addressed. _is_safe_url() resolves via getaddrinfo, then session.get(...) resolves DNS independently a second time; attacker DNS can return a public IP for the check and 169.254.169.254 for the fetch. Robust fix is resolve-once-then-pin (connect to the validated IP with the original Host header). If you don't want to implement pinning now, at minimum add a comment marking it as a known limitation so it isn't mistaken for airtight.

3. No test added. Per CLAUDE.md PR expectations, new features need a CPU unit test. A pytest for _is_blocked_ip / _is_safe_url (localhost, 169.254.169.254, IPv6 loopback ::1, a file:// scheme, a normal https URL) plus a mocked-redirect case (mock socket.getaddrinfo since _is_safe_url does live DNS) would lock in behavior — and would have caught the first-hop-only gap above.

🟢 Minor

• Likely lint failure: import order (image.py:17). from urllib.parse import urlparse, urljoin isn't alphabetized (urljoin sorts before urlparse); pre-commit run --all-files will likely rewrite it. Please run pre-commit before pushing.
• is_reserved still not in _is_blocked_ip (image.py:34-40). Misses 240.0.0.0/4 etc. Cheap to add alongside the others.
• IPv4-mapped IPv6 not unwrapped. Low practical risk (getaddrinfo without AI_V4MAPPED returns real A/AAAA records), but unwrapping ip.ipv4_mapped before classifying makes _is_blocked_ip correct regardless of caller.
• startswith("http") (image.py:84) still matches httpfoo://; harmless since _is_safe_url re-checks the parsed scheme, but matching the parsed scheme reads cleaner.

Net: the redirect hook fires now (real improvement), but it guards only hop 1 — a 2+ hop chain to the metadata endpoint still gets through. I'd switch to the manual allow_redirects=False loop so every hop is validated pre-fetch, then close out DNS-rebinding (or document it) and add the unit test.

(I couldn't run requests in this sandbox to demonstrate the multi-hop path at runtime; the finding is from the requests API — SessionRedirectMixin.resolve_redirects is a single generator that loops the whole chain via an internal while url: with self.send(..., allow_redirects=False). Worth a quick local confirmation with a 2-hop redirect.)

(CLA still needs signing per the bot comment above before this can merge.)
• branch fix/ssrf-image-decoder